Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor config to load on init #202

Merged
merged 23 commits into from
Jul 24, 2023
Merged

refactor config to load on init #202

merged 23 commits into from
Jul 24, 2023

Conversation

bradleyandrick
Copy link
Contributor

@bradleyandrick bradleyandrick commented Jul 14, 2023 •
edited
Loading

Related Issue(s):

  • NA

Proposed Changes:

  1. move config location from src/assets/config.js to ./public/config/config.json
  2. change config to be a json object instead of separate exported constants
  3. Load config.json into redux on app load once instead of of direct imports
  4. Add pre-initialization page to handle and show error (and not render app) if config is missing
  5. Change config key names to remove the word VITE (leftover from when they were .env vars)
  6. Refactor tests for search to be single file now
  7. Refactor default.js file to only be shared, static values (also clean up dead code here)

BONUS: add leaflet-draw types & fixed a11y error in launchModal

To Test:

  • confirm unit tests pass 🍏

Test new config

  • see /public/config/config.example.json for new format
  • add director named config with file namedconfig.json to /public folder

NOTE: see ticket for config.json that can be used in testing

  • update config params to match existing file constants
  • remove old local config.js file so it does not get committed by accident if adding new commits
  • npm run start like normal to load app
  • app should have no regressions after changes made (manual spot check to confirm)

Test no config

  • delete or change name of new config.json file
  • reload app
  • confirm app does not fully load child components
  • page should be blank dark mode screen
  • confirm error message displays as expected

PR Checklist:

  • I have added my changes to the CHANGELOG or a CHANGELOG entry is not required.

@bradleyandrick bradleyandrick marked this pull request as ready for review July 14, 2023 22:03
@bradleyandrick bradleyandrick changed the title DRAFT: refactor config to load on init refactor config to load on init Jul 14, 2023
philvarner
philvarner reviewed Jul 15, 2023
View reviewed changes
README.md Outdated
| Variable | Description | Required |
| ------------------------ | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------- |
| PUBLIC_URL | URL for the FilmDrop UI. Useful when using a CDN to host application. | Optional |
| VITE_APP_NAME | Name for this app. (set in `.env`, because it is needed prior to any JS loading) | Optional |
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should split this table into the one config needed in .env and the others used in config.json

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also move the two required config variables to the top of the list. Also wondering if it would be worth handling the behavior when the scene tiler isn't defined more gracefully so that it's not required.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I've not split the table into two, one for .env and one for config.json
  • I've moved the required var to the top (only one now)
  • I've removed the required flag for the SCENE_TILER_URL var since it is generally gracefully handled after refactor from consolidate map functions out of search.js component. I've only needed to remove the console log, not if SCENE_TILER_URL not set, it will still highlight footprint but close loading spinner and not try to load overlay

philvarner
philvarner reviewed Jul 15, 2023
View reviewed changes
src/services/get-config-service.js
import { showApplicationAlert } from '../utils/alertHelper'

export async function LoadConfigIntoStateService() {
await fetch(`/config/config.json`, {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this going to work right if the ui is deployed somewhere other than the root, e.g., if it's deployed at http://www.example.com/foo instead of http://www.example.com/ ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes it should work just fine since the fetch is using a relative URL path that starts with /. This means it will request the file from the root of the domain, regardless of what path the React app is deployed to

@bradleyandrick bradleyandrick merged commit c718626 into main Jul 24, 2023
@bradleyandrick bradleyandrick deleted the ba/refactor-config-to-load-on-init branch August 2, 2024 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@philvarner philvarner philvarner approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bradleyandrick @philvarner